[BOLT] Check entry point address is not in constant island #163418
Conversation
Asher8118
requested review from
aaupov,
ayermolo,
maksfb,
paschalis-mpeis,
rafaelauler,
yota9 and
yozhu
as code owners
|
@llvm/pr-subscribers-bolt Author: Asher Dobrescu (Asher8118) ChangesThere are cases where Full diff: https://github.com/llvm/llvm-project/pull/163418.diff 2 Files Affected:
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index dd0d041692484..c35775464751b 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -1336,8 +1336,17 @@ void BinaryContext::processInterproceduralReferences() {
<< Function.getPrintName() << " and "
<< TargetFunction->getPrintName() << '\n';
}
- if (uint64_t Offset = Address - TargetFunction->getAddress())
- TargetFunction->addEntryPointAtOffset(Offset);
+ if (uint64_t Offset = Address - TargetFunction->getAddress()) {
+ if (!TargetFunction->isInConstantIsland(Address)) {
+ TargetFunction->addEntryPointAtOffset(Offset);
+ } else {
+ TargetFunction->setIgnored();
+ this->outs() << "BOLT-WARNING: Ignoring entry point at address 0x"
+ << Twine::utohexstr(Address)
+ << " in constant island of function " << *TargetFunction
+ << '\n';
+ }
+ }
continue;
}
diff --git a/bolt/test/AArch64/constant-island-entry.s b/bolt/test/AArch64/constant-island-entry.s
new file mode 100644
index 0000000000000..bd0906d87f15f
--- /dev/null
+++ b/bolt/test/AArch64/constant-island-entry.s
@@ -0,0 +1,35 @@
+// This test checks that we ignore functions which add an entry point that
+// is in a costant island.
+
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-linux-gnu %s -o %t.o
+# RUN: %clang %cflags %t.o -pie -Wl,-q -o %t.exe
+# RUN: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s
+
+# CHECK: BOLT-WARNING: Ignoring entry point at address 0x{{[0-9a-f]+}} in constant island of function func
+
+.globl func
+.type func, %function
+func:
+ ret
+ nop
+ b .Lafter_constant
+
+.type constant_island, %object
+constant_island:
+ .xword 0xabcdef
+
+.Lafter_constant:
+ ret
+ .size func, .-func
+
+.globl caller
+.type caller, %function
+caller:
+ bl constant_island
+ ret
+
+.globl main
+.type main, %function
+main:
+ bl caller
+ ret
|
There was a problem hiding this comment.
Hey Ash,
Thanks for the patch! IIUC, your use case is an entrypoint to a code island, which was marked as data?
Given we branch to it, I'm not sure if we are confident enough to use this as a heuristic, treating such cases as code and processing them fully, rather than ignoring the functions? If input code is indeed problematic, it'll crash regardless of being ignored or not.
I could look into this in the future, however for this case it's not straightforward to check if we can process the area marked as data rather than ignore the function.
This should not be the case, if we are dealing with code marked as data then the input binary itself is not problematic, it's only BOLT that struggles to parse it. |
There was a problem hiding this comment.
Thanks Ash! Yes, I was curious about an easy win; probably not as easy as #160161. But not a blocker for this patch, since BOLT would assert/crash anyway.
If input code is indeed problematic, it'll crash regardless of being ignored or not.
This should not be the case, if we are dealing with code marked as data then the input binary itself is not problematic, it's only BOLT that struggles to parse it.
To clarify: yes, that matches your use case.
I was referring to a truly problematic input binary though, ie an island not meant to be code or containing a bad instruction sequence.
In such cases, whether we ignore it and proceed (as in this patch), or use the branching as a heuristic, convert the marker and process it, it would crash anyway, but that wouldn't be BOLT's responsibility.
Looks good! Give it a day or two in case there are any further comments.
4 participants
There are cases where
addEntryPointAtOffsetis called with a givenOffsetthat points to an address within a constant island. This triggersassert(!isInConstantIsland(EntryPointAddress)and causes BOLT to crash. This patch adds a check which ignores functions that would add such entry points and warns the user.